Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.3] Add test to proove issue with numeric aggregation #14991

Closed
wants to merge 2 commits into from
Closed

[5.3] Add test to proove issue with numeric aggregation #14991

wants to merge 2 commits into from

Conversation

segadora
Copy link
Contributor

I dont know if this is the place to place the test.

I am having issues with min() and max() on the eloquent builder is expected to be a float or int.

I have a scenario, where I am interrested in seeing the highest and lowest datetime in the database.

The problem seems to have been implemented in following pull request:
#14793

/**
* Get a database connection instance.
*
* @return Illuminate\Database\Connection
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing leading slash (please only added it to phpdoc, and not to code)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file is not in a namespace. Therefor it should not be needed? Or am I wrong?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our phpdoc requires a leading slash.

@segadora
Copy link
Contributor Author

@GrahamCampbell is this considered a bug or do you want me to make everything pretty first?

EloquentBuilderTestUser::create(['id' => 1, 'email' => 'test1@test.test', 'created_at' => '2016-08-10 09:21:00']);
EloquentBuilderTestUser::create(['id' => 2, 'email' => 'test2@test.test', 'created_at' => '2016-08-01 12:00:00']);

$this->assertEquals('2016-08-10 09:21:00', EloquentBuilderTestUser::max('created_at'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not numeric.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method only returns ints and floats.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GrahamCampbell I know. But isn't it wrong?

If I cant use the max('date') on the QueryBuilder on a datetime field. How do you want me to get the maximum date then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this method never claimed to support doing things like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you are saying that I will have to make some kind of raw SQL to archive what has been possible before the numeric update?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I think the phpdoc said that it could return a string or mixed before. The behavior has not changed.

If you are at larecon I won't mind discussing it with you

@GrahamCampbell
Copy link
Member

Thanks for the PR. Please could you send to 5.1 so we can merge it along with #14994.

@segadora segadora mentioned this pull request Aug 24, 2016
taylorotwell pushed a commit that referenced this pull request Aug 24, 2016
* Add test
Add test to see if min and max can be used on datetime type of fields
Refs. #14991

* Remove leading slashes

* Fix phpdoc class reference

* Add leading slash
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants